[GVN] Turn off ScalarPRE for TokenLike Types #156513

farzonl · 2025-09-02T18:41:03Z

In HLSL the GVNPass was adding a phi node on
a target extention type.
https://hlsl.godbolt.org/z/sc14YenEe

This is something we cleaned up in a past PR (#154620) by introducing isTokenLikeTy. In the case of the GVN pass the target extention type was still making its way through. This change makes it so if we see this type we don't do PRE.

fixes llvm#154407 In HLSL the GVNPass was adding a phi node on a target extention type. https://hlsl.godbolt.org/z/sc14YenEe This is something we cleaned up in a past PR (llvm#154620) by introducing `isTokenLikeTy`. In the case of the GVN pass the target extention type was still making its way through. This change makes it so if we see this type we don't do PRE.

bogner

I really wish there was an easier way to make it clear that an update_test_checks test is just checking that the output exactly matches the output. Might be worth commenting as such in the note that explains what this is doing.

The test can also be simplified quite a bit - I commented on some of the particular things to do that, but since I modified it myself to come up with those comments anyway I think it more or less boils down to this:

define i32 @f(i32 %x) {
entry:
  %cmp = icmp eq i32 %x, 0
  br i1 %cmp, label %exit, label %for.body.lr.ph

for.body.lr.ph:
  %buf = call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding(i32 0, i32 0, i32 4, i32 %x, ptr nonnull @Out.str)
  br label %for.body

for.body:
  %i = phi i32 [ 0, %for.body.lr.ph ], [ %inc.i, %for.body ]
  %v1 = call i32 @llvm.dx.resource.updatecounter(target("dx.RawBuffer", i32, 1, 0) %buf, i8 1)
  %inc.i = add nuw nsw i32 %i, 1
  %exitcond = icmp ne i32 %inc.i, %x
  br i1 %exitcond, label %for.body, label %exit

exit:
  %buf2 = call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding(i32 0, i32 0, i32 4, i32 %x, ptr nonnull @Out.str)
  %v2 = call i32 @llvm.dx.resource.updatecounter(target("dx.RawBuffer", i32, 1, 0) %buf2, i8 1)

  ret i32 %v2
}

bogner · 2025-09-02T19:13:24Z

llvm/test/Transforms/GVN/PRE/no-phi-translate.ll

+; CHECK-NEXT:    ret void
+;
+entry:
+  %0 = tail call i32 @llvm.dx.flattened.thread.id.in.group()


We don't really need flattened.thread.id.in.group to test this, could just use an i32 argument to the function

bogner · 2025-09-02T19:13:47Z

llvm/test/Transforms/GVN/PRE/no-phi-translate.ll

+CSMain.exit.loopexit:                         ; preds = %for.body.i
+  br label %CSMain.exit


This block is unnecessary

bogner · 2025-09-02T19:15:09Z

llvm/test/Transforms/GVN/PRE/no-phi-translate.ll

+  br i1 %cmp.i1.not, label %CSMain.exit, label %for.body.i.lr.ph
+
+for.body.i.lr.ph:                                 ; preds = %entry
+  %1 = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_i32_1_0t(i32 0, i32 0, i32 4, i32 %0, ptr nonnull @Out.str)


Please give all of the values names (ie, %buf instead of %1 here) to make tests easier to edit in the future. Also, we don't need to use the overload in the name, might as well let the IR parser do that and just call @llvm.dx.resource.handlefrombinding(...)

bogner · 2025-09-02T19:15:55Z

llvm/test/Transforms/GVN/PRE/no-phi-translate.ll

+  %5 = tail call noundef nonnull align 4 dereferenceable(4) ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) %3, i32 0)
+  store i32 %4, ptr %5, align 4
+  ret void


We don't really need the getpointer and the store here - we can just ret i32 %4 (though also please give %4 a name

bogner

Looks good to me apart from the test improvements.

farzonl · 2025-09-02T19:21:32Z

llvm/test/Transforms/GVN/PRE/no-phi-translate.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=gvn %s | FileCheck %s
+
+; NOTE: when we use a Token like type we should not introduce a phi


@bogner i did put this note in

This note makes sense - I was saying it might also be good to note that in this case we expect GVN not to do anything at all, which gives some context for reading what the check lines are checking.

So maybe something like "Test that GVN doesn't introduce phis for token like types. NOTE: This implies that GVN won't do anything at all to this input". OTOH maybe the existing comment is clear enough. I'll leave it to you to decide.

hekota

LGTM with one small suggestion.

hekota · 2025-09-02T21:07:32Z

llvm/test/Transforms/GVN/PRE/no-phi-translate.ll

+
+; NOTE: A test to confirm GVN doesn't introduce phis for token like types.
+; NOTE: This implies the CHECKS should exactly match the IR.
+


Suggested change

; CHECK-NOT: phi

there can be phi nodes just not on Token like types. This would likely break the phi in the for loop. I think what exist is sufficent because if a phi were to show up later this test would fail and then someone would come read this note.

llvmbot · 2025-09-03T16:04:20Z

@llvm/pr-subscribers-backend-directx

@llvm/pr-subscribers-llvm-transforms

Author: Farzon Lotfi (farzonl)

Changes

fixes #154407

In HLSL the GVNPass was adding a phi node on
a target extention type.
https://hlsl.godbolt.org/z/sc14YenEe

This is something we cleaned up in a past PR (#154620) by introducing isTokenLikeTy. In the case of the GVN pass the target extention type was still making its way through. This change makes it so if we see this type we don't do PRE.

Full diff: https://github.com/llvm/llvm-project/pull/156513.diff

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/GVN.cpp (+2-1)
(added) llvm/test/Transforms/GVN/PRE/no-phi-translate.ll (+51)

diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 4baa3b3eb8242..26e17cc849bff 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -2982,7 +2982,8 @@ bool GVNPass::performScalarPREInsertion(Instruction *Instr, BasicBlock *Pred,
 bool GVNPass::performScalarPRE(Instruction *CurInst) {
   if (isa<AllocaInst>(CurInst) || CurInst->isTerminator() ||
       isa<PHINode>(CurInst) || CurInst->getType()->isVoidTy() ||
-      CurInst->mayReadFromMemory() || CurInst->mayHaveSideEffects())
+      CurInst->mayReadFromMemory() || CurInst->mayHaveSideEffects() ||
+      CurInst->getType()->isTokenLikeTy())
     return false;
 
   // Don't do PRE on compares. The PHI would prevent CodeGenPrepare from
diff --git a/llvm/test/Transforms/GVN/PRE/no-phi-translate.ll b/llvm/test/Transforms/GVN/PRE/no-phi-translate.ll
new file mode 100644
index 0000000000000..d330ec46f9905
--- /dev/null
+++ b/llvm/test/Transforms/GVN/PRE/no-phi-translate.ll
@@ -0,0 +1,51 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=gvn %s | FileCheck %s
+
+; NOTE: A test to confirm GVN doesn't introduce phis for token like types.
+; NOTE: This implies the CHECKS should exactly match the IR.
+
+%"$Globals" = type { i32 }
+@CBV = external constant %"$Globals"
+@Out.str = private unnamed_addr constant [4 x i8] c"Out\00", align 1
+
+define i32 @CSMain() local_unnamed_addr {
+; CHECK-LABEL: define i32 @CSMain() local_unnamed_addr {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[LOADGLOBAL:%.*]] = load i32, ptr @CBV, align 4
+; CHECK-NEXT:    [[CMP_I1_NOT:%.*]] = icmp eq i32 [[LOADGLOBAL]], 0
+; CHECK-NEXT:    br i1 [[CMP_I1_NOT]], label %[[CSMAIN_EXIT:.*]], label %[[FOR_BODY_I_LR_PH:.*]]
+; CHECK:       [[FOR_BODY_I_LR_PH]]:
+; CHECK-NEXT:    [[BUF:%.*]] = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_i32_1_0t(i32 0, i32 0, i32 4, i32 [[LOADGLOBAL]], ptr nonnull @Out.str)
+; CHECK-NEXT:    br label %[[FOR_BODY_I:.*]]
+; CHECK:       [[FOR_BODY_I]]:
+; CHECK-NEXT:    [[LOOPPHI:%.*]] = phi i32 [ 0, %[[FOR_BODY_I_LR_PH]] ], [ [[INC_I:%.*]], %[[FOR_BODY_I]] ]
+; CHECK-NEXT:    [[UPDATECNT:%.*]] = tail call noundef i32 @llvm.dx.resource.updatecounter.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) [[BUF]], i8 1)
+; CHECK-NEXT:    [[INC_I]] = add nuw nsw i32 [[LOOPPHI]], 1
+; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp ne i32 [[INC_I]], [[LOADGLOBAL]]
+; CHECK-NEXT:    br i1 [[EXITCOND]], label %[[FOR_BODY_I]], label %[[CSMAIN_EXIT]]
+; CHECK:       [[CSMAIN_EXIT]]:
+; CHECK-NEXT:    [[BUFEXIT:%.*]] = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_i32_1_0t(i32 0, i32 0, i32 4, i32 [[LOADGLOBAL]], ptr nonnull @Out.str)
+; CHECK-NEXT:    [[UPDATECNTEXIT:%.*]] = tail call noundef i32 @llvm.dx.resource.updatecounter.tdx.RawBuffer_i32_1_0t(target("dx.RawBuffer", i32, 1, 0) [[BUFEXIT]], i8 1)
+; CHECK-NEXT:    ret i32 [[UPDATECNTEXIT]]
+;
+entry:
+  %loadGlobal = load i32, ptr @CBV, align 4
+  %cmp.i1.not = icmp eq i32 %loadGlobal, 0
+  br i1 %cmp.i1.not, label %CSMain.exit, label %for.body.i.lr.ph
+
+for.body.i.lr.ph:
+  %buf = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding(i32 0, i32 0, i32 4, i32 %loadGlobal, ptr nonnull @Out.str)
+  br label %for.body.i
+
+for.body.i:
+  %loopPhi = phi i32 [ 0, %for.body.i.lr.ph ], [ %inc.i, %for.body.i ]
+  %updateCnt = tail call noundef i32 @llvm.dx.resource.updatecounter(target("dx.RawBuffer", i32, 1, 0) %buf, i8 1)
+  %inc.i = add nuw nsw i32 %loopPhi, 1
+  %exitcond = icmp ne i32 %inc.i, %loadGlobal
+  br i1 %exitcond, label %for.body.i, label %CSMain.exit
+
+CSMain.exit:
+  %bufExit = tail call target("dx.RawBuffer", i32, 1, 0) @llvm.dx.resource.handlefrombinding(i32 0, i32 0, i32 4, i32 %loadGlobal, ptr nonnull @Out.str)
+  %updateCntExit = tail call noundef i32 @llvm.dx.resource.updatecounter(target("dx.RawBuffer", i32, 1, 0) %bufExit, i8 1)
+  ret i32 %updateCntExit
+}

farzonl requested a review from nikic September 2, 2025 18:41

farzonl added this to HLSL Support Sep 2, 2025

farzonl added the backend:DirectX label Sep 2, 2025

bogner reviewed Sep 2, 2025

View reviewed changes

bogner approved these changes Sep 2, 2025

View reviewed changes

farzonl commented Sep 2, 2025

View reviewed changes

update test case to address pr comments.

e887729

farzonl force-pushed the bugfix/issue-154407 branch from 74057c8 to e887729 Compare September 2, 2025 20:37

hekota approved these changes Sep 2, 2025

View reviewed changes

farzonl merged commit 49fcfaa into llvm:main Sep 2, 2025
9 checks passed

github-project-automation bot moved this to Closed in HLSL Support Sep 2, 2025

farzonl added the llvm:transforms label Sep 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[GVN] Turn off ScalarPRE for TokenLike Types #156513

[GVN] Turn off ScalarPRE for TokenLike Types #156513

Uh oh!

farzonl commented Sep 2, 2025

Uh oh!

bogner left a comment

Uh oh!

bogner Sep 2, 2025

Uh oh!

bogner Sep 2, 2025

Uh oh!

bogner Sep 2, 2025

Uh oh!

bogner Sep 2, 2025

Uh oh!

bogner left a comment

Uh oh!

farzonl Sep 2, 2025

Uh oh!

bogner Sep 2, 2025

Uh oh!

hekota left a comment

Uh oh!

hekota Sep 2, 2025

Uh oh!

farzonl Sep 2, 2025

Uh oh!

Uh oh!

llvmbot commented Sep 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

		CSMain.exit.loopexit: ; preds = %for.body.i
		br label %CSMain.exit


		; NOTE: A test to confirm GVN doesn't introduce phis for token like types.
		; NOTE: This implies the CHECKS should exactly match the IR.

[GVN] Turn off ScalarPRE for TokenLike Types #156513

[GVN] Turn off ScalarPRE for TokenLike Types #156513

Uh oh!

Conversation

farzonl commented Sep 2, 2025

Uh oh!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hekota left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Sep 3, 2025 •

edited

Loading